Skip to content

Hotfix for issues 251 and 261#262

Merged
Gui-FernandesBR merged 3 commits intomasterfrom
hotfix-261-251
Oct 24, 2022
Merged

Hotfix for issues 251 and 261#262
Gui-FernandesBR merged 3 commits intomasterfrom
hotfix-261-251

Conversation

@giovaniceotto
Copy link
Copy Markdown
Member

@giovaniceotto giovaniceotto commented Oct 22, 2022

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

See #205, #251 and #261.

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

See Colab at: https://colab.research.google.com/drive/1viSLeMVux1aaxqRhXTQ6Ubid7qG0-wUi?usp=sharing

@giovaniceotto giovaniceotto self-assigned this Oct 22, 2022
@giovaniceotto giovaniceotto added the Bug Something isn't working label Oct 22, 2022
@giovaniceotto
Copy link
Copy Markdown
Member Author

giovaniceotto commented Oct 22, 2022

@MrGribel, I am a bit worried about the new verison requirement for netCDF4. What do you think happens if someone has a version above 1.6.0 and tries to update rocketpy?

@MrGribel
Copy link
Copy Markdown
Contributor

MrGribel commented Oct 23, 2022

Congrats on the fast response @giovaniceotto

Addressing your question, I've had netCDF4-1.6.1 installed on my local machine, and pip uninstalled it before installing the older netCDF4-1.5.8 when given the new requirements file, which I assume the behavior you intended.

It worries me a bit that we are introducing a dependency on an older version of a package though (I've had experiences with these getting removed at times from PyPI, for example). I believe a better long term fix would be to figure out what's going on inside the netCDF4 library. Perhaps keep the issue #205 open for the time being.

Besides that, the rest of the changes seem like pretty straightforward fixes. Thanks for that!

Gui-FernandesBR
Gui-FernandesBR approved these changes Oct 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again, sir!

@Gui-FernandesBR Gui-FernandesBR merged commit a9f9fab into master Oct 24, 2022
@Gui-FernandesBR Gui-FernandesBR deleted the hotfix-261-251 branch October 26, 2022 19:23
@Gui-FernandesBR Gui-FernandesBR mentioned this pull request Nov 5, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gui-FernandesBR Gui-FernandesBR Gui-FernandesBR approved these changes

@MateusStano MateusStano Awaiting requested review from MateusStano

+1 more reviewer

@MrGribel MrGribel MrGribel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@giovaniceotto giovaniceotto

Labels

Bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@giovaniceotto @MrGribel @Gui-FernandesBR